HDDS-15864. Branch merge checklist for Short Circuit read - #503
HDDS-15864. Branch merge checklist for Short Circuit read#503ChenSammi wants to merge 3 commits into
Conversation
Gargi-jais11
left a comment
There was a problem hiding this comment.
Thanks @ChenSammi for preparing this. LGTM!
|
|
||
| ## 2. Documentation | ||
|
|
||
| [User Documentation](https://github.com/apache/ozone/blob/HDDS-10685/hadoop-hdds/docs/content/feature/Short-Circuit-Read.md) of Short Circuit Read has been added. |
There was a problem hiding this comment.
@ChenSammi @Gargi-jais11
This PR completely removes the old website and Hugo since it is now unused. So I suggest to use https://ozone.apache.org/docs/next/administrator-guide/configuration/performance/short-circuit-local-reads
There was a problem hiding this comment.
Thanks @ChenSammi for the working on the checklist. Please check the inline comment. Also we may need to create followup Jira to fix urls that pointing to feature branch code to master branch after merge.
I plan to merge this patch after feature branch is merged, so URL can be updated later. |
priyeshkaratha
left a comment
There was a problem hiding this comment.
Thanks @ChenSammi for clarifying comments. Changes LGTM
errose28
left a comment
There was a problem hiding this comment.
Thanks for adding this. Left some comments inline.
|
|
||
| ## 9. Possible incompatible changes/used feature flag | ||
|
|
||
| Short-circuit read is gated by the non-rolling upgrade framework as HDDSLayoutFeature.SHORT_CIRCUIT_READS (layout version 11). |
There was a problem hiding this comment.
Why is the layout feature necessary? It looks like this just creates a socket and closes it on shutdown. How would this break on downgrade?
Also it looks like new clients can request short circuit read from an old server it will be ignored. This seems fine since the field is phrased as "request" but that is worth noting in this section too.
There was a problem hiding this comment.
It's to avoid a new client talk to an old server. Yes, the new client has fallback mechanism, but it will cost time.
There was a problem hiding this comment.
Then this is not correct and was not tested. It is always the newest component's responsibility to handle compatibility, and Datanode layout feature is never communicated to the client so it is impossible for a new client to do anything about this with the current implementation.
If we actually want the new client to do some handling with an old server, the layout feature should be removed and a DatanodeVersion needs to be added instead with a check on the client side. However, SCR may not even be enabled on the server side regardless of its version and finalization status, so we would need a transparent fallback anyways. The DatanodeVersion would just save a retry as a regular read for the new client case. It looks like a LayoutFeature is not required either way since I don't see any downgrade implications.
The two version system is confusing, that's why we've reduced it to a single version in ZDU so this type of thing should be easier to follow going forward.
There was a problem hiding this comment.
Datanode layout feature is never communicated to the client so it is impossible for a new client to do anything about this with the current implementation
Client knows the Datanode version and checks it.
XceiverClientManager L211.
if (OzoneNetUtils.isAddressLocal(addr) &&
dn.getCurrentVersion() >= SHORT_CIRCUIT_READS.toProtoValue()) {
There was a problem hiding this comment.
Ok this is good but the doc doesn't mention a new DatanodeVersion was also added. Looks like action items are:
- Update the doc to mention the new
DatanodeVersionto handle the new client case. - Remove the added
HDDSLayoutFeaturefrom the branch since there doesn't appear to be any downgrade concerns.
|
|
||
| End-to-end read performance is therefore dominated by local disk I/O, similar to a direct file read, rather than by gRPC data transfer. | ||
|
|
||
| A benchmark was run against feature branch HDDS-10685, and get this result https://docs.google.com/document/d/1xmjCbK4rP287pPN0_iOonnQee6HgwSI3h5hVbtgfu3k/edit?tab=t.0. |
There was a problem hiding this comment.
This doc isn't very large, we can put its content directly in this file. We should not commit links to google docs which can disappear or lose access to git.
There was a problem hiding this comment.
Can you convert the images to markdown tables so they show up in text searches and can have their content copied? If google doc's "copy as markdown" doesn't support this then AI should make this easy.
There was a problem hiding this comment.
I already tried convert the image to markdown table first with Cursor. But cursor did not give me a nice table as I expected after several round of interaction, so finally it's converted to png image.
There was a problem hiding this comment.
@errose28 , do you know how we can make this new engine allows words like YCSB and ycloud?
docs/08-developer-guide/04-project/01-git/03-feature-branches/03-merged-branches/17-hdds-10685-short-circuit-read.md:80:122 - Unknown word (YCSB) -- ozone fs` command and YCSB on a ycloud cluster
docs/08-developer-guide/04-project/01-git/03-feature-branches/03-merged-branches/17-hdds-10685-short-circuit-read.md:80:132 - Unknown word (ycloud) -- command and YCSB on a ycloud cluster. Short-circuit
There was a problem hiding this comment.
@ChenSammi u can add the unknown words coming up in this file cspell.yaml. Then the error will be gone.
There was a problem hiding this comment.
+1 to the above, or inline cspell ignores can be added if the word is only relevant for this page. In this case YCSB is probably valid to allow globally in cspell.yaml.
The instructions in the contributing guide are also linked from the CI failure itself:
...
CSpell: Files checked: 1347, Issues found: 5 in 1 file.
Checking file names...
CSpell: Files checked: 1, Issues found: 0 in 0 files.
Spell check failed. See CONTRIBUTING.md for help fixing false positive spelling errors:
https://github.com/apache/ozone-site/blob/master/CONTRIBUTING.md#spelling
There was a problem hiding this comment.
Google docs "copy as markdown" was able to create the markdown tables. They render well in docusaurus too. I've put this and the spelling fix on my fork here. You can cherry-pick these into this PR.
Note I added "YCSB" to cspell where previously it was wrapped in code blocks. Although spelling is ignored for code blocks YCSB is not used as code here, and code blocks as headings render badly in docusaurus right now.
|
|
||
| Short-circuit communication uses a UNIX domain socket (`ozone.domain.socket.path`) between the client and Datanode. | ||
|
|
||
| It follows the same [Hadoop Socket Path Security](https://cwiki.apache.org/confluence/spaces/HADOOP2/pages/120730260/SocketPathSecurity) rules as HDFS short-circuit reads. |
There was a problem hiding this comment.
agreed, as a TODO item we should copy this to our doc site.
There was a problem hiding this comment.
https://cwiki.apache.org/confluence/spaces/HADOOP2/pages/120730260/SocketPathSecurity content is included in the design document "Security" section.
There was a problem hiding this comment.
Let's update this section to reference our own user or design docs with this content instead of linking to the hadoop wiki.
|
|
||
| ## 2. Documentation | ||
|
|
||
| [User Documentation](https://github.com/apache/ozone/blob/HDDS-10685/hadoop-hdds/docs/content/feature/Short-Circuit-Read.md) of Short Circuit Read has been added. |
There was a problem hiding this comment.
the feature would require topology awareness to maximize locality https://ozone.apache.org/docs/next/administrator-guide/configuration/performance/topology/#optimizing-read-paths
let's update user doc post merge.
|
|
||
| End-to-end read performance is therefore dominated by local disk I/O, similar to a direct file read, rather than by gRPC data transfer. | ||
|
|
||
| A benchmark was run against feature branch HDDS-10685, testing short-circuit performance with the `ozone fs` command and YCSB on a ycloud cluster. Short-circuit read was toggled with `ozone.client.read.short-circuit` and `ozone.domain.socket.path`. |
There was a problem hiding this comment.
please replace ycloud with "Cloudera's internal test bed"
Move benchmark images under static/img, escape YCSB for spell check, and replace internal cluster name with plain-language wording. Co-authored-by: Cursor <cursoragent@cursor.com>
What changes were proposed in this pull request?
Add branch merge checklist for short-circuit read.
What is the link to the Apache Jira?
HDDS-15864
How was this patch tested?